-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(browser): Add shim package for logs #18831
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
size-limit report 📦
|
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
| consoleSandbox(() => { | ||
| // eslint-disable-next-line no-console | ||
| console.warn('You are using consoleLoggingIntegration() even though this bundle does not include logs.'); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm I know we also do this in other shims but I'm wondering if we should instead guard these logs with DEBUG_BUILD. My thinking is:
- We export dedicated
.debugbundle variants - When using the loader, users can toggle debug logging
- End users should probably not get these warnings in their console, just because our users decided to not use the feature. This is especially relevant for the loader use case.
- This helps keep the CDN bundle size increase at a minimum, since we can tree-shake out the logs for the normal bundles
WDYT? Am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that makes sense, I'll update that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
...ges/browser-integration-tests/suites/public-api/logger/consoleLoggingIntegrationShim/test.ts
Show resolved
Hide resolved
Lms24
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Adds a shim for the
loggernamespace since we'll soon ship the logs bundle in the loader and want to avoid breaking user apps when they switch back to a bundle that does not include logscloses #18826